Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[java] for loop enhance and using of standard java 11 method writeString for tests #14889

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

[java] for loop enhance and using of standard java 11 method writeString for tests #14889

wants to merge 5 commits into from

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Dec 11, 2024 •
edited by qodo-merge-pro bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This pull request includes several changes to improve the code by replacing the Files.write method with the more concise Files.writeString method. Additionally, there is a minor refactor to simplify a loop structure.

Codebase simplification:

Method replacement:

Motivation and Context

I changed the method calls for writing strings to standard ones from Java 11 to include more standard solutions from the language library. I also replaced looping through the collection with an iterator for better code readability.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement

Description

  • Refactored for loops in MBean.java to use enhanced for-loop syntax for improved readability.
  • Replaced Files.write with Files.writeString across multiple files to simplify file writing operations.
  • Removed unnecessary charset specifications in file writing methods.
  • Enhanced code simplicity and readability by using modern Java methods.

Changes walkthrough 📝

Relevant files
Enhancement
MBean.java
Refactor for loop to enhanced for-loop syntax                       

java/src/org/openqa/selenium/grid/jmx/MBean.java

  • Refactored a for loop to use enhanced for-loop syntax.
  • Improved readability by simplifying loop structure.
    		•  +3/-3     
    DockerSessionFactory.java
    Use Files.writeString for writing strings to files             

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java

  • Replaced Files.write with Files.writeString.
  • Simplified file writing process.
    		•  +2/-3     
    UploadTest.java
    Simplify file writing with Files.writeString                         

    java/test/org/openqa/selenium/UploadTest.java

  • Updated file writing to use Files.writeString.
  • Removed unnecessary charset specification.
    		•  +1/-2     
    EdgeOptionsTest.java
    Update file writing to Files.writeString                                 

    java/test/org/openqa/selenium/edge/EdgeOptionsTest.java

  • Changed file writing method to Files.writeString.
  • Improved code simplicity and readability.
    		•  +1/-1     
    AppServerTestBase.java
    Use Files.writeString for temporary file creation               

    java/test/org/openqa/selenium/environment/webserver/AppServerTestBase.java

  • Replaced Files.write with Files.writeString.
  • Simplified temporary file creation.
    		•  +1/-2     
    NodeTest.java
    Simplify file writing in NodeTest with Files.writeString 

    java/test/org/openqa/selenium/grid/node/NodeTest.java

  • Updated multiple instances of file writing to Files.writeString.
  • Removed charset specification for simplicity.
    		•  +2/-3     
    ResourceHandlerTest.java
    Refactor file operations to use Files.writeString               

    java/test/org/openqa/selenium/grid/web/ResourceHandlerTest.java

  • Replaced Files.write with Files.writeString in tests.
  • Enhanced readability and simplicity of file operations.
    		•  +3/-4     
    TestUtilities.java
    Simplify file writing in TestUtilities                                     

    java/test/org/openqa/selenium/testing/TestUtilities.java

  • Simplified file writing by using Files.writeString.
  • Removed unnecessary charset usage.
    		•  +1/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @iampopovich iampopovich marked this pull request as ready for review December 11, 2024 18:46
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    File Permission:
    The code creates temporary files with default permissions which could potentially be accessed by other users on the system. Consider setting appropriate file permissions when creating temporary files containing potentially sensitive data like session capabilities.

    ⚡ Recommended focus areas for review

    Bug
    The createFile method writes content to the directory path instead of the newly created file path, which will cause file corruption or errors

    Error Handling
    The method should close file resources properly in case of exceptions during file writing operations

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix critical bug where content is written to wrong file path

    Fix incorrect file path usage - currently writing to directory path instead of the
    newly created file path.

    java/test/org/openqa/selenium/grid/node/NodeTest.java [919]

    -Files.writeString(directory.toPath(), content);
+Files.writeString(f.toPath(), content);
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: This is a critical bug fix where content is being written to the directory path instead of the newly created file path, which would cause file operation failures and potential data corruption.

    		10
    General
    Use explicit character encoding instead of platform-dependent default charset to ensure consistent file writing

    Specify a charset explicitly instead of using the default charset to ensure
    consistent encoding across different platforms and environments.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [457-458]

     Files.writeString(
-    Paths.get(path, "sessionCapabilities.json"), capsToJson, Charset.defaultCharset());
+    Paths.get(path, "sessionCapabilities.json"), capsToJson, StandardCharsets.UTF_8);
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using explicit UTF-8 encoding instead of platform-dependent default charset is a good practice to ensure consistent behavior across different environments. While not critical, it prevents potential encoding issues.

    		6

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @iampopovich @pujagani